Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f55e3a931
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| accountRateLimits: activeRateLimits, | ||
| usageShowRemaining: appSettings.usageShowRemaining, | ||
| accountInfo: activeAccount, | ||
| accountWorkspaceLabel: activeWorkspace?.name ?? null, |
There was a problem hiding this comment.
These props are wired from activeRateLimits/activeAccount, but on the Home screen we clear activeWorkspaceId (src/features/workspaces/hooks/useWorkspaceSelection.ts), and MainApp.tsx derives both values from activeWorkspaceId, so they become null in the exact context where this dashboard is shown. As a result, Home never gets rate-limit/account data and accountCards stays empty, so the new “Account limits” section does not render in normal Home usage; this should be sourced from the workspace selection used by Home (or another home-specific account source) instead of active-thread workspace state.
Useful? React with 👍 / 👎.
|
Not ready yet... don’t merge 😁 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03a974bae2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| aggregateHomeAccountSelection.isCommitted && | ||
| canRetainAggregateHomeAccountWorkspaceId( | ||
| aggregateHomeAccountSelection.workspaceId, | ||
| workspaces, | ||
| rateLimitsByWorkspace, |
There was a problem hiding this comment.
When Home is in “All workspaces” mode and no workspace has a usable account/rate-limit snapshot yet (common right after startup or when auth returns placeholder data), this retention gate fails and stableHomeAccountWorkspaceId falls back to resolveHomeAccountWorkspaceId on each thread-activity reorder. That makes the selected account workspace bounce between projects and repeatedly re-triggers the refresh effect (refreshAccountInfo/refreshAccountRateLimits) for different workspaces, creating avoidable RPC churn and unstable Home/Sidebar account data until one workspace happens to return usable data.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03a974bae2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return workspaceHasAccountData( | ||
| workspace, | ||
| rateLimitsByWorkspace, | ||
| accountByWorkspace, | ||
| ); |
There was a problem hiding this comment.
In aggregate Home mode (usageWorkspaceId === null), canRetainAggregateHomeAccountWorkspaceId only checks whether cached account/rate-limit data exists and ignores workspace.connected. If the previously selected workspace disconnects, this returns true, so stableHomeAccountWorkspaceId keeps pointing at a disconnected workspace and the refresh effect no longer runs (homeAccountWorkspace?.connected gate), leaving stale limits visible even though another connected workspace is available. This conflicts with resolveHomeAccountWorkspaceId, which explicitly prioritizes connected workspaces.
Useful? React with 👍 / 👎.
| value: formatCompactNumber(usageTotals?.last30DaysTokens ?? last7Tokens), | ||
| suffix: "tokens", | ||
| caption: `Total ${formatCount(usageTotals?.last30DaysTokens ?? last7Tokens)}`, |
There was a problem hiding this comment.
The "Last 30 days" token card falls back to last7Tokens when usageTotals?.last30DaysTokens is unavailable, so partial/older snapshot payloads underreport the 30-day total as a 7-day value. Since this component already defensively handles missing totals fields, this fallback should use a 30-day aggregate from usageDays rather than the 7-day sum.
Useful? React with 👍 / 👎.
|
@Dimillian I think we are good to go on this one. |
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Feature PR adding usage insights to the home dashboard. The test coverage is commendably deep — 529 lines of hook tests plus Sidebar tests for the credits display. A few issues to flag:
Logic concerns
resolveHomeAccountWorkspaceId fallback ordering: When usageWorkspaceId is set but doesn't match any workspace (the "missing" case in tests), the function falls back to a heuristic. The tests assert specific ordering (connected > disconnected, most recent activity, real account over unknown type) but the production fallback logic isn't visible in this diff. It's important that the ranking is deterministic when two workspaces have identical updatedAt timestamps — undefined sort order here could cause the home dashboard to flicker between workspaces across renders.
inferredHasCredits in normalizeCreditsSnapshot: The new inference logic sets hasCredits: true when normalizedBalance is a non-empty string. This means a balance of "0" would set hasCredits: true, which is semantically wrong — zero credits means no usable credits. Consider changing the inference to parseFloat(normalizedBalance) > 0 or only relying on the explicit hasCredits field. The existing keeps credit balances visible when unlimited is explicitly false test uses balance "120" so it wouldn't catch this edge case.
Numeric balance normalization: typeof balanceRaw === "number" && Number.isFinite(balanceRaw) won't handle NaN (which passes typeof === "number" but fails isFinite). The guard is correct but the else branch falls through to previousCredits?.balance ?? null, which is probably fine — just worth an explicit test for balance: NaN to document the intended behavior.
Test gaps
- No test for
balance: "0"— the inferredhasCreditsbug described above. - No test for concurrent workspace refresh (both
refreshAccountInfoandrefreshAccountRateLimitscalled in the same tick) to verify no double-render or stale closure issues. - The
Sidebar.test.tsxsnapshot test for credits checksscreen.getByText("Available credits: 120")— if the number formatting ever changes (e.g., locale-aware"120.00"or""), this will silently break. Consider a more resilient matcher.
CSS
The is-up / is-down gradient backgrounds look fine. One potential issue: var(--text-accent) for the is-up color — if that's a blue accent on a blue-tinted gradient background (rgba(88, 190, 255, 0.18)), the contrast ratio may fall below WCAG AA. Worth a quick accessibility check with the actual theme colors.
Minor
- The screenshot in
docs/screenshots/is committed as a binary file — if this directory isn't gitignored, it'll bloat the repo history on every dashboard update. Consider linking to an external asset or using git-lfs.
Overall: good feature with strong test coverage. The inferredHasCredits zero-balance edge case and deterministic workspace sorting are the highest-priority issues before merge.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 01433c46a7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return workspaceHasAccountData( | ||
| workspace, | ||
| rateLimitsByWorkspace, | ||
| accountByWorkspace, | ||
| ); |
There was a problem hiding this comment.
The retention guard for committed aggregate selection only checks whether cached account/rate-limit data exists, not whether that workspace is still connected. In the All workspaces flow, this can lock the Home account source onto a disconnected workspace (for example after startup with stale cached data), and because refresh is skipped for disconnected workspaces, account limits can stay stale even after another workspace connects and has current data.
Useful? React with 👍 / 👎.
2 participants
Summary
Testing
npm run typechecknpm run testScreenshot